[rolling fallback] incentive dutch auction - #1127
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35cc852e60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| require( | ||
| blueMarketParams.collateralToken == midnightMarket.collateralParams[collateralIndex].token, | ||
| InconsistentCollateralToken() | ||
| ); | ||
| require(midnightMarket.collateralParams[collateralIndex].lltv <= blueMarketParams.lltv, BlueLltvTooLow()); |
There was a problem hiding this comment.
these checks (as well as the loan token check) are not perfect because the users could set non-valid configs
There was a problem hiding this comment.
potential solution, check these in setConfig and use a midnight and blue market instead of their respective ids (it would be more expensive in calldata though)
There was a problem hiding this comment.
it's not too bad that one can set an invalid config, but IMO we should do either all checks in setConfig (meaning you can't enable an invalid config) or all checks in roll
There was a problem hiding this comment.
Imho it's better to do all check in setConfig to prevent a user from setting something invalid by accident, feeling safe, and then getting liquidated because rolling was not possible.
There was a problem hiding this comment.
there are currently 4 checks on a given config: start <= end, incentives <= WAD, loan token match and collateral token match. We can't really check the collateral tokens because the config might be set before the user supply any collateral into midnight. It's also more expensive in gas to check the loan token in setConfig because in the current version we only use the market Ids. So I slightly favor the current version
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e54c34760
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
chrmatt
left a comment
chrmatt
left a comment
There was a problem hiding this comment.
Seems there is no test case to check that having a LLTV on Blue that is lower than the one on Midnight reverts.
Also added some comments. Otherwise looks good.
| require( | ||
| blueMarketParams.collateralToken == midnightMarket.collateralParams[collateralIndex].token, | ||
| InconsistentCollateralToken() | ||
| ); | ||
| require(midnightMarket.collateralParams[collateralIndex].lltv <= blueMarketParams.lltv, BlueLltvTooLow()); |
There was a problem hiding this comment.
Imho it's better to do all check in setConfig to prevent a user from setting something invalid by accident, feeling safe, and then getting liquidated because rolling was not possible.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13104de76e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: MathisGD <74971347+MathisGD@users.noreply.github.com> Signed-off-by: PA <50184410+peyha@users.noreply.github.com>
3 participants
No description provided.